build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

LuigiImVector · 2022-07-13T18:04:54Z

What does this PR do?

Improve repo

For resources

Description

I improved the fpb-lint.yml workflow so that linter errors were shown via comment, as proposed by @SethFalco on #4416.

In this PR LuigiImVector#10 I conducted some tests, all successful.

Checklist:

Read our contributing guidelines

Follow-up

Check the status of GitHub Actions and resolve any reported warnings!

…ion#4416

.github/workflows/fpb-lint.yml

Co-authored-by: David Ordás <[email protected]>

.github/workflows/fpb-lint.yml

Co-authored-by: David Ordás <[email protected]>

.github/workflows/fpb-lint.yml

SethFalco · 2022-07-13T18:58:38Z

The problem from my PR is that if you look at your tests, you'll notice you executed all of those comments from a workflow in the PR.

If given write access to the repo, the PR could easily be modified by a malicious actor to do whatever they want, including spam or execute other actions.

You can see this happening already if you read the error.

failed to create review: Message: Resource not accessible by integration, Locations: [{Line:1 Column:66}]

The solution to this is to use pull_request_target if I understand correctly, but that will open up a whole can of worms.

I recommend you read this:
Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

In the docs, they create an unprivileged workflow to process the files and create the message as a file and upload it as an artifact. Then in a separate workflow that is privileged, they fetch that artifact and post the review.

This is where I got too busy and put this off, though. Sorry about that! ^-^'

LuigiImVector · 2022-07-13T19:14:37Z

Before we merge this, would you mind merging the one in your fork, and perhaps I can try to do a malicious PR to it, and we'll see what happens? 🤔

Now I'm going to read the article you linked, in the meantime is it enough for me to write?
on: [push, pull_request_target]

I'm not sure if I understand correctly

SethFalco · 2022-07-13T19:16:50Z

Now I'm going to read the article you linked, in the meantime is it enough for me to write?

That'd work, I think.

But that's exactly what you shouldn't do! Because then others can trigger modify the GitHub Action with write-access to the repo.

In other words, it'd be a vulnerability.

TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

— Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

SethFalco · 2022-07-13T19:18:58Z

I recommend you scroll down to where it says:

Below is an example of the intended usage in which the results of an unprivileged pull_request workflow are combined with a privileged workflow to leave a comment in response to a received PR:

— Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

You'll see an example of 2 workflows used to leave comments or reviews without opening the vulnerability. The thing you want to avoid is having write permissions in a workflow that checks out the repo.

Workflow 1 (Normal, no write access to repo.)

Lint the source
Create the message
Save message to a file
Upload file as artifact

Workflow 2 (Privileged, with write access to repo and no checkout action.)

Download artifact
Extract zip
Pipe contents of file to review

It's possible I may have misunderstood something, so if anyone else can chime in, feel free.

LuigiImVector · 2022-07-13T19:41:10Z

Over here you'll see an example of it using 2 workflows to be able to leave comments/reviews without creating the security issue.

"Incoming data from artifacts is potentially untrusted. When used in a safe manner, like reading PR numbers or reading a code coverage text to comment on the PR, it is safe to use such untrusted data in the privileged workflow context. However if the artifacts were, for example, binaries built from an untrusted PR, it would be a security vulnerability to run them in the privileged workflow_run workflow context. Artifacts resulting from untrusted PR data are themselves untrusted and should be treated as such when handled in privileged contexts."

Even this could not be a totally safe solution

Maybe it's enougth to disable the auto-run of the workflow, giving us the ability to approve only non-suspicious PR

SethFalco · 2022-07-13T19:45:40Z

When used in a safe manner, like reading PR numbers or reading a code coverage text to comment on the PR, it is safe to use such untrusted data in the privileged workflow context

However if the artifacts were, for example, binaries built from an untrusted PR, it would be a security vulnerability to run them in the privileged workflow_run workflow context

You're right, but in this case, it's fine because we're just reading a text file (1st quote), not executing a binary (2nd quote).

We could opt to disable auto-run as well, but I'm thinking we should be fine with the 2 workflow solution. GitHub themselves state it's fine so long as we don't execute untrusted binaries, which we're not doing.

In our case, we're just reading a text file that we ourselves created. The worst thing that could happen is someone intentionally writes something unfavorable to the file, which wouldn't be much different from them just leaving that comment themselves.

LuigiImVector · 2022-07-13T19:53:58Z

So I'll try to implement the 2 workflows here

LuigiImVector · 2022-07-13T21:16:03Z

I divided the workflow and now the free-programming-books-lint doesn't any return error; workflow_run is only triggered in the default branch

Here is a test: LuigiImVector#8 (review)

SethFalco

I've tested all the suggestions in this repo:
https://github.com/SethFalco/free-programming-books/pull/6

There is another issue. Since we have 2 workflows named free-programming-books-lint, one for push and one for pull-request, the comment action gets triggered twice, with one always failing since the artifacts don't exist.

I don't think we really need this on push anyway, so perhaps we can drop that one and make this run on pull-request only?

Giving it more thought, I originally proposed that we should have the action approve or request. However, I think that might be a bad idea, and we should instead only post a comment.

Since we have many other checks in this repository in other actions, it'd look odd if this action approved the pull request, while other linter actions failed. Contributors will receive contradicting information and won't be sure whether they need to fix something or not. We should probably just post a comment, and later we can try to hide it once it's outdated.

This can be done with the following change:

-             gh pr review $(<PRurl) -r -b "Linter failed, fix the error(s):
+             gh pr comment $(<PRurl) -b "Linter failed, fix the error(s):
              \`\`\`
              $(cat error.log)
              \`\`\`"
              gh pr edit $(<PRurl) --add-label "linter error"
            else
-             gh pr review $(<PRurl) -a

SethFalco · 2023-10-20T14:48:44Z

.github/workflows/fpb-lint.yml

+        fpb-lint ./books/ &>> output.log || echo "Analyzing..."
+        fpb-lint ./casts/ &>> output.log || echo "Analyzing..."
+        fpb-lint ./courses/ &>> output.log || echo "Analyzing..."
+        fpb-lint ./more/ &>> output.log || echo "Analyzing..."


Suggested change

fpb-lint ./books/ &>> output.log || echo "Analyzing..."

fpb-lint ./casts/ &>> output.log || echo "Analyzing..."

fpb-lint ./courses/ &>> output.log || echo "Analyzing..."

fpb-lint ./more/ &>> output.log || echo "Analyzing..."

fpb-lint books casts courses more &> output.log

The latest release of fpb-lint allows this to be done in one command.

I don't think it's worthwhile to echo Analyzing, since it will be performed after it's already finished anyway.

SethFalco · 2023-10-20T14:50:15Z

.github/workflows/fpb-lint.yml

+        fpb-lint ./books/
+        fpb-lint ./casts/
+        fpb-lint ./courses/
+        fpb-lint ./more/


Suggested change

fpb-lint ./books/

fpb-lint ./casts/

fpb-lint ./courses/

fpb-lint ./more/

fpb-lint books casts courses more

SethFalco · 2023-10-21T13:50:39Z

.github/workflows/fpb-lint.yml

+    - name: Clean output
+      if: ${{ always() && 
+          github.event_name == 'pull_request' }}
+      uses: actions/github-script@v6
+      with:
+        script: |
+          const fs = require('fs');
+          const readline = require('readline');
+
+          const file = readline.createInterface({
+            input: fs.createReadStream('output.log'),
+            output: process.stdout,
+            terminal: false,
+          });
+
+          let lastLine = '';
+          file.on('line', (line) => {
+            if (lastLine) {
+              fs.appendFile('error.log', lastLine, (err) => {
+                if (err) {
+                  console.error(err);
+                }
+              });
+            }
+
+            if (line.includes('/home/runner/work/free-programming-books/')) {
+              lastLine = line.replace('/home/runner/work/free-programming-books/', '') + "\r\n"; 
+            } else if (line.includes('\u26a0')) {
+              lastLine = '\r\n\r\n';
+            } else if (line.includes('remark-lint')) {
+              lastLine = line + '\r\n';
+            } else {
+              lastLine = null;
+            }
+          });
+
+          file.on('close', () => {
+            if (!lastLine || lastLine === '\r\n\r\n') {
+              return;
+            }
+
+            fs.appendFile('error.log', lastLine, (err) => {
+              if (err) {
+                console.error(err);
+              }
+            });
+          });
+
+    - name: Upload artifact
+      if: ${{ always() && 
+          github.event_name == 'pull_request' }}
+      run: |
+          mkdir -p ./pr
+          echo ${{ github.event.pull_request.html_url }} > ./pr/PRurl
+          mv error.log ./pr/error.log


Suggested change

- name: Clean output

if: ${{ always() &&

github.event_name == 'pull_request' }}

uses: actions/github-script@v6

with:

script: |

const fs = require('fs');

const readline = require('readline');

const file = readline.createInterface({

input: fs.createReadStream('output.log'),

output: process.stdout,

terminal: false,

});

let lastLine = '';

file.on('line', (line) => {

if (lastLine) {

fs.appendFile('error.log', lastLine, (err) => {

if (err) {

console.error(err);

}

});

}

if (line.includes('/home/runner/work/free-programming-books/')) {

lastLine = line.replace('/home/runner/work/free-programming-books/', '') + "\r\n";

} else if (line.includes('\u26a0')) {

lastLine = '\r\n\r\n';

} else if (line.includes('remark-lint')) {

lastLine = line + '\r\n';

} else {

lastLine = null;

}

});

file.on('close', () => {

if (!lastLine || lastLine === '\r\n\r\n') {

return;

}

fs.appendFile('error.log', lastLine, (err) => {

if (err) {

console.error(err);

}

});

});

- name: Upload artifact

if: ${{ always() &&

github.event_name == 'pull_request' }}

run: |

mkdir -p ./pr

echo ${{ github.event.pull_request.html_url }} > ./pr/PRurl

mv error.log ./pr/error.log

- name: Clean output and create artifacts

if: ${{ always() &&

github.event_name == 'pull_request' }}

run: |

mkdir -p ./pr

echo ${{ github.event.pull_request.html_url }} > ./pr/PRurl

cat output.log | sed -E 's:/home/runner/work/free-programming-books/|⚠.+::' | uniq > ./pr/error.log

I think this would be a lot simpler with bash instead of JavaScript, what do you think? The suggestion should produce the same output. Furthermore, we can merge it with the Upload artifact task below now that, since the cleanup is just a single line.

Command Description

cat output.log Get the contents of output.log.

sed -E 's:/home/runner/work/free-programming-books/|⚠.+::' Replace /home/runner/work/free-programming-books/ and the warning symbol (⚠) and all characters after it until the end of the line with nothing.

uniq Remove redundant line breaks.

SethFalco · 2023-10-21T20:39:50Z

.github/workflows/comment-pr.yml

+  GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+
+jobs:
+  upload:


Suggested change

upload:

upload:

permissions:

pull-requests: write

This job would need to explicitly declare write access for pull-request here, I think?
Either that, or in the repo settings someone would have to enable write access by default for all workflows, but that's probably not a good idea for a public repo.

SethFalco · 2023-10-21T20:41:51Z

.github/workflows/fpb-lint.yml

+
+        touch error.log


Suggested change

touch error.log

If my other suggestions are merged, these line can be deleted.

build(fpb-lint): linting errors as PR comments, resolved EbookFoundat…

7568a64

…ion#4416

LuigiImVector added the New Feature New feature / enhancement / translation... label Jul 13, 2022

davorpa assigned SethFalco and LuigiImVector Jul 13, 2022

davorpa reviewed Jul 13, 2022

View reviewed changes

.github/workflows/fpb-lint.yml Outdated Show resolved Hide resolved

davorpa linked an issue Jul 13, 2022 that may be closed by this pull request

Linting Errors as PR Comments #4416

Closed

build(fpb-lint): removed unused code

1007933

Co-authored-by: David Ordás <[email protected]>

davorpa reviewed Jul 13, 2022

View reviewed changes

.github/workflows/fpb-lint.yml Outdated Show resolved Hide resolved

build(fpb-lint): edited command to create an empty file

35b4081

Co-authored-by: David Ordás <[email protected]>

davorpa reviewed Jul 13, 2022

View reviewed changes

.github/workflows/fpb-lint.yml Outdated Show resolved Hide resolved

build: use two different workflows

f16f1e4

LuigiImVector requested a review from SethFalco July 15, 2022 07:32

davorpa added the 🤖 automation Automated tasks done by workflows or bots label Jul 15, 2022

davorpa mentioned this pull request Aug 10, 2022

ci(workflow): post build result as comment #5564

Closed

2 tasks

LuigiImVector mentioned this pull request Sep 2, 2022

Custom github app #7054

Open

This comment was marked as outdated.

Sign in to view

github-actions bot added the stale Requests that have not had recent interaction (Out-of-Date) label Sep 14, 2022

davorpa added 📌 pinned Issues that are pinned currently keep Requests exempt from being punctually stale and removed stale Requests that have not had recent interaction (Out-of-Date) 📌 pinned Issues that are pinned currently labels Sep 14, 2022

SethFalco reviewed Oct 21, 2023

View reviewed changes

SethFalco mentioned this pull request Oct 21, 2023

ci(workflow): post build result as comment #10618

Merged

2 tasks

eshellman closed this in #10618 Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

LuigiImVector commented Jul 13, 2022

SethFalco commented Jul 13, 2022 •

edited

Loading

LuigiImVector commented Jul 13, 2022

SethFalco commented Jul 13, 2022 •

edited

Loading

SethFalco commented Jul 13, 2022 •

edited

Loading

LuigiImVector commented Jul 13, 2022

SethFalco commented Jul 13, 2022 •

edited

Loading

LuigiImVector commented Jul 13, 2022

LuigiImVector commented Jul 13, 2022 •

edited

Loading

This comment was marked as outdated.

SethFalco left a comment •

edited

Loading

SethFalco Oct 20, 2023

SethFalco Oct 20, 2023

SethFalco Oct 21, 2023 •

edited

Loading

SethFalco Oct 21, 2023

SethFalco Oct 21, 2023

Command	Description
`cat output.log`	Get the contents of `output.log`.
`sed -E 's:/home/runner/work/free-programming-books/\|⚠.+::'`	Replace `/home/runner/work/free-programming-books/` and the warning symbol (`⚠`) and all characters after it until the end of the line with nothing.
`uniq`	Remove redundant line breaks.

-  upload:
+  upload:
+    permissions:
+      pull-requests: write

build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

build(fpb-lint): linting errors as PR comments, resolved #4416 #6914

Conversation

LuigiImVector commented Jul 13, 2022

What does this PR do?

For resources

Description

Checklist:

Follow-up

SethFalco commented Jul 13, 2022 • edited Loading

LuigiImVector commented Jul 13, 2022

SethFalco commented Jul 13, 2022 • edited Loading

SethFalco commented Jul 13, 2022 • edited Loading

Workflow 1 (Normal, no write access to repo.)

Workflow 2 (Privileged, with write access to repo and no checkout action.)

LuigiImVector commented Jul 13, 2022

SethFalco commented Jul 13, 2022 • edited Loading

LuigiImVector commented Jul 13, 2022

LuigiImVector commented Jul 13, 2022 • edited Loading

This comment was marked as outdated.

SethFalco left a comment • edited Loading

Choose a reason for hiding this comment

SethFalco Oct 20, 2023

Choose a reason for hiding this comment

SethFalco Oct 20, 2023

Choose a reason for hiding this comment

SethFalco Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

SethFalco Oct 21, 2023

Choose a reason for hiding this comment

SethFalco Oct 21, 2023

Choose a reason for hiding this comment

SethFalco commented Jul 13, 2022 •

edited

Loading

SethFalco commented Jul 13, 2022 •

edited

Loading

SethFalco commented Jul 13, 2022 •

edited

Loading

SethFalco commented Jul 13, 2022 •

edited

Loading

LuigiImVector commented Jul 13, 2022 •

edited

Loading

SethFalco left a comment •

edited

Loading

SethFalco Oct 21, 2023 •

edited

Loading